Skip to content

Deprecate types.loaOf#63103

Merged
worldofpeace merged 3 commits intoNixOS:masterfrom
rnhmjoj:loaof
Jan 6, 2020
Merged

Deprecate types.loaOf#63103
worldofpeace merged 3 commits intoNixOS:masterfrom
rnhmjoj:loaof

Conversation

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jun 13, 2019

Motivation for this change

Fix issue #1800.

It turns out this is a lot of work: there are many instances of lists being used instead of sets.
The main culprits are users.users and environment.etc.
I think I have identified most of them but there are still some, moreover all this changes should be properly tested so I definitely need some help.

@rnhmjoj rnhmjoj requested review from edolstra and nbp as code owners June 13, 2019 22:35
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 13, 2019
@infinisil
Copy link
Member

If we want to deprecate this, there needs to be some deprecation warning for users. We can't just flip the switch and possibly break every second person's config. A good way to do that is to change the loaOf type to issue a warning when the list case is used, something like

Warning: You're assigning the list [ { name = "foo"; bar = <CODE>; ... } ... ] to
<option name>, which won't be supported in the future, use an attribute set like
{ foo = { bar = <CODE>; ... }; ... } instead. See
https://github.com/NixOS/nixpkgs/pull/63103 for more info.

@rnhmjoj rnhmjoj force-pushed the loaof branch 3 times, most recently from 4c0cfbd to 2412d3b Compare June 14, 2019 08:10
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jun 14, 2019

Ok, I removed the attempts to replace loaOf with attrsOf and added a deprecation warning.
I don't know how to do something like that though

lib/types.nix Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this isn't what we need. The warning needs to go in the merge function. There one has access to all definitions, which can then be filtered to lists only, and for each of them one can print a warning as I showed in my comment. would be even neater to include the file and line where the user needs to do this change.

I generally think our error messages suck too much. It's not too difficult to make them better, especially with all the info the module system gives access to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took me a while but I think this should do it:

trace: warning: In file <nixpkgs/nixos/modules/services/misc/nix-daemon.nix>
a list is being assigned to the option config.users.users.
This will soon be an error as type loaOf is deprecated.
See https://git.io/fj2zm for more information.
Do
  users.users =
    { nixbld1 = {...}; nixbld2 = {...}; nixbld3 = {...}; ...}
instead of
  users.users =
    [ { name = "nixbld1"; ...} { name = "nixbld2"; ...} { name = "nixbld3"; ...} ...]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, no it doesn't work when the set is missing the name attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, it does but it's not pretty.

@rycee
Copy link
Member

rycee commented Jun 14, 2019

This will break the programs.ssh.matchBlocks option in Home Manager. That option allows use of list when the order matters and an attribute set as a convenience for users whose setup does not require a certain order.

I believe I originally had it as a listOf but got complaints that it was cumbersome for simple setups.

I don't particularly mind switching back to listOf since I, in principle, agree about the rationale for deprecating loaOf but thought I'd make a note of this use-case.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jun 15, 2019

@rycee I don't know how home-manager works: are you somehow bound to nixos option types? Couldn't you simply define an equivalent loaOf for home-manager?

@rnhmjoj rnhmjoj force-pushed the loaof branch 2 times, most recently from ef6ce8b to fdab0a3 Compare June 15, 2019 21:58
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 15, 2019
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 16, 2019
@rnhmjoj rnhmjoj force-pushed the loaof branch 3 times, most recently from 5d7cfe4 to 57440d5 Compare June 17, 2019 15:54
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 17, 2019
@rnhmjoj rnhmjoj mentioned this pull request Jul 28, 2019
@worldofpeace worldofpeace added 9.needs: changelog This PR needs a changelog entry and removed 2.status: work-in-progress labels Jan 6, 2020
@aanderse
Copy link
Member

aanderse commented Jan 6, 2020

Thank you everyone, especially @rnhmjoj for your hard work!! 🎉

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Jan 6, 2020

If docbook inconveniences you, I can translate your markups if that would be convenient for you.

No, it's not a problem: I have written documentation for NixOS before.

If you could make the note similar to how our warning works in types.nix, explains how and why loaOf is deprecated and how code should be changed to not use it. And what phase of deprecation we're in, I believe it's not removed and code that uses it should work as it has.

Ok, I'll do something like it. Thank you.

@ghost
Copy link

ghost commented Jan 7, 2020

I think there should be a pinned issue for this, since it will throw warnings and errors for many people and the efforts to fix those should be coordinated somehow.

@worldofpeace
Copy link
Contributor

I think there should be a pinned issue for this, since it will throw warnings and errors for many people and the efforts to fix those should be coordinated somehow.

Sounds good to me, will open.

@worldofpeace
Copy link
Contributor

Made a little pop up issue #77189. Should have relevant strings within the body to pop up in relevancy searches.

@rnhmjoj rnhmjoj deleted the loaof branch January 8, 2020 17:28
mebubo added a commit to mebubo/dotfiles that referenced this pull request Jan 9, 2020
@jtojnar
Copy link
Member

jtojnar commented Jan 11, 2020

The deprecation should probably be documented in nixos/doc/manual/development/option-types.xml.

infinisil added a commit to infinisil/nixpkgs that referenced this pull request Jan 12, 2020
@grahamc
Copy link
Member

grahamc commented Jan 14, 2020

I just want to say I got the most wonderful error message from a Nix evaluation today:

trace: warning: In file [...]/packet-spot-buildkite-agent/network.nix
a list is being assigned to the option config.programs.ssh.knownHosts.
This will soon be an error as type loaOf is deprecated.
See https://git.io/fj2zm for more information.
Do
  programs.ssh.knownHosts =
    { flexo.gsc.io = {...}; }
instead of
  programs.ssh.knownHosts =
    [ { hostNames = [ "flexo.gsc.io" "r13y.com" "147.75.105.137" ]; ...} ]

This error message:

✔️ is specific: "a list is being assigned to the option config.programs.ssh.knownHosts"

✔️ tells me where to go for more information: https://git.io/fj2zm

✔️ tells me where the violation is: .../packet-spot-buildkite-agent/network.nix

✔️ SHOWS me the code which is wrong:

      programs.ssh.knownHosts =
        [ { hostNames = [ "flexo.gsc.io" "r13y.com" "147.75.105.137" ]; ...} ]

✔️ AND tells me the solution:

      programs.ssh.knownHosts =
        { flexo.gsc.io = {...}; }

This is incredible, and ticks the main three boxes of good errors:

  1. What is the problem?
  2. Why did it happen?
  3. How do I fix it?

Way to go everyone, this is a massive step forward for our community and I am completely floored and delighted. Thank you.

@aanderse aanderse mentioned this pull request May 5, 2020
10 tasks
@infinisil infinisil mentioned this pull request Sep 4, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: printing Drivers, CUPS & Co. 6.topic: qt/kde Object-oriented framework for GUI creation 8.has: module (update) This PR changes an existing module in `nixos/` 9.needs: changelog This PR needs a changelog entry 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants